Add new_context to instrumentation #1499
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
tests/test_logfire.py
Outdated
| tagged = logfire.with_tags('test_instrument') | ||
|
|
||
| def context_factory(): | ||
| return {'new_contex': 123} |
There was a problem hiding this comment.
We didn't discuss this context factory API, I'm not sure if I like it or understand the point. It's certainly not being tested here, new_contex doesn't do anything or appear in the exported spans.
There was a problem hiding this comment.
I didn't mention it in the original issue because it wasn't something I'd considered. But when I realized I'd just be using attach_context behind the scenes, it seemed preferable to allow users the option to provide input for attach_context.
If you're against it, it's not that useful for me and I don't mind cutting it. But if it's worth keeping I'll add better testing for it.
alexmojaki
left a comment
alexmojaki
left a comment
There was a problem hiding this comment.
this feature isn't really compelling enough without the span link
I'll look at adding it, just trying to determine the best approach. |
2 participants
As per #1493, it's current a bit awkward to remove context from instrumentation. This PR adds the "new_context" keyword to ensure designated function calls do not inherit from parent spans.
Here is an example from the new test:
Alternatively, new_context can be set to
True, which is the same asnew_context=dict.